Benchmarking Compute Units #6
Conversation
buffalojoec
force-pushed
the
bench-compute
branch
from
a3b5a41 to
0194e29
Compare
buffalojoec
force-pushed
the
bench-compute
branch
2 times, most recently
from
9de22ab to
94cd34c
Compare
buffalojoec
force-pushed
the
bench-compute
branch
from
94cd34c to
d1b7f73
Compare
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Looks really great overall! Just some small things
| bench_program_compute_units: | ||
| name: Benchmark Program Compute Units | ||
| runs-on: ubuntu-latest | ||
| needs: build_programs # Cargo Bench won't build the SBPF binary... |
There was a problem hiding this comment.
Looks like we need cargo bench-sbf 😅
| '--features', | ||
| 'bpf-entrypoint', |
There was a problem hiding this comment.
🙏 thank you for doing this
| fn max_space() -> u64 { | ||
| bincode::serialized_size(&Self::default()).unwrap() | ||
| } |
There was a problem hiding this comment.
Up to you, but this function is the same for all implementations, so you may as well define it in the trait
There was a problem hiding this comment.
Unfortunately this is from the ConfigState trait, which is required for the instruction helper store. I could just not use the helper, though.
There was a problem hiding this comment.
Ah gotcha, this can stay as is, no worries!
| /// Convert to a `Bench`. | ||
| pub fn bench(&self) -> Bench { | ||
| (self.label.as_str(), &self.instruction, &self.accounts) | ||
| } |
There was a problem hiding this comment.
nit for mollusk-svm-bencher: but it would be clearer to have Bench be a struct
This is great! |
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Two last tiny things, everything else looks great!
program/tests/functional.rs
Outdated
| + key_len * entry_size | ||
| let total_keys_size = (key_len).checked_mul(entry_size).unwrap(); | ||
| bincode::serialized_size(&(ConfigKeys::default(), MyConfig::default())) | ||
| .ok() |
There was a problem hiding this comment.
nit: is the ok() needed? I thought you could just do and_then on the Result: https://doc.rust-lang.org/std/result/enum.Result.html#method.and_then
There was a problem hiding this comment.
I only did it because checked_add returns an option, so it was one less unwrap(). I suppose it's all the same, though.
program/tests/functional.rs
Outdated
| @@ -1,4 +1,4 @@ | |||
| #![cfg(feature = "test-sbf")] | |||
| // #![cfg(feature = "test-sbf")] | |||
There was a problem hiding this comment.
Why was this commented out?
Benchmarking compute unit usage for the BPF implementation of the
Config program using Mollusk.
Mollusk's CU bencher is designed to profile compute unit usage for a series
of benchmark instructions.
The bencher will only write a new table to the markdown file if CU usage has
changed. With this in mind, we can integrate CU benching into our CI so that
if there is ever a change to the program that affects CUs, the file must be
committed with the new benchmarks.